Skip to content
This repository has been archived by the owner on Jan 18, 2023. It is now read-only.

STIP 006 - Trade Rebates #12

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

STIP 006 - Trade Rebates #12

wants to merge 10 commits into from

Conversation

richardliang
Copy link
Contributor

No description provided.

Copy link
Contributor

@bweick bweick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused what the goal is with these fees, is this what we want? Don't we actually want something that tracks potential rebates based on the volume traded? The we'd charge the protocol fee as normal but payback a portion of that to the manager?

STIPS/STIP-005.md Outdated Show resolved Hide resolved
@richardliang richardliang changed the title STIP 005 - Trade Rebates STIP 006 - Trade Rebates Oct 12, 2021
@richardliang
Copy link
Contributor Author

I'm a little confused what the goal is with these fees, is this what we want? Don't we actually want something that tracks potential rebates based on the volume traded? The we'd charge the protocol fee as normal but payback a portion of that to the manager?

Added option 4 which is governance chooses the protocol fee and a rebate split %

#### Recommendation
Option 1 will require the least code changes while giving us the ability to emulate fee rebates. The maxManagerFee can be enforced either on initialization (still susceptible to manager rugging by removing and reinitializing) or stored on the Controller by governance
#### Future work
Tiered rebates: With this base rebate mechanism, manager contracts can be built on top in the future. Individual managers will always have the ability to specify a fee recipient address as their own, but manager contracts for a specific product (e.g. social trading) can abstract away the fee recipient to a shared peripheral contract. This central trading contract tracks cumulative volume done through trading all the Sets that are linked, and perform calculations that split the rebates collected. This way, at the base module level, rebates are flat across Sets but at the manager contract level, rebates can be tiered
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing I'd add here as potential future feature is potentially updating the TradeModule to allow each Set to have its own rebate percentage (instead of a global one for now). This could be negotiated with large managers/applications that use the protocol.

### TradeModuleV2
- Inherit TradeModule
- Override `trade`
- Add `virtual` to TradeModule V1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think you have to do this. Would rather not touch the V1 code.

- Override `trade`
- Add `virtual` to TradeModule V1
- Add `_accrueManagerFee`
- Override constructor to add `managerRebateRecipient`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean initialize?

- Override `trade` and `tradeRemainingWETH`
- Add `virtual` to GeneralIndexModule trade functions
- Add `_accrueManagerFee`
- Override constructor to add `managerRebateRecipient`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments here as above

- Inherit TradeModule
- Override `trade` to update ComponentExchanged event
- Override `_accrueProtocolFee`
- Override initialize to add `managerRebateRecipient`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it'll be this simple since this will require a different interface so isn't a clean override, more of an overload. Also we'll have to track this state somewhere.

#### Public Variables
| Type | Name | Description |
|------ |------ |------------- |
|mapping(ISetToken => address);|managerRebateRecipient|Mapping of SetToken to address of rebate recipient|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

> initialize(SetToken _setToken, address _managerRebateRecipient) external
- managerRebateRecipient: manager rebate recipient address
```solidity
function initialize(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting this will also require a change in the initialize function on the GIMExtension

> function _accrueProtocolFee(TradeInfo memory _tradeInfo, uint256 _exchangedQuantity) internal returns (uint256, uint256)
- No changes to interface
```solidity
function _accrueProtocolFee(TradeInfo memory _tradeInfo, uint256 _exchangedQuantity) internal returns (uint256 protocolFeeTotal, uint256 managerRebateTotal) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

override

> function _accrueProtocolFee(TradeInfo memory _tradeInfo) internal returns (uint256, uint256)
- No changes to interface
```solidity
function _accrueProtocolFee(TradeInfo memory _tradeInfo) internal returns (uint256 protocolFeeTotal, uint256 managerRebateTotal) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

override

@bweick
Copy link
Contributor

bweick commented Oct 13, 2021

My main concern is that initialize isn't easily override-able since we will not have the same interface. We'll be over-loading that function which we don't want to do otherwise if someone calls the initialize(address) version all the rebates will end up being sent to the blackhole

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants